Phase G ops suite: audit, 2FA, telemetry, schedule, tunnel#9
Conversation
- Add internal/audit leaf: an Auditor seam (Begin/End + Record helper, nil = no-op) and a JSONL FileAuditor that appends who/what/when and the outcome for each destructive operation. Write errors are swallowed — auditing must never fail the operation it records. - Add a single interception point in the app layer (guardedOp): it runs an authorization Gate (no-op until the 2FA cycle implements it) and opens the audit record, returning a done(err) the verb defers. Wire it into Backup, Restore, Sync, and Prune. This is the shared seam the 2FA and telemetry cycles reuse rather than re-wrapping every verb. - Add Deps.Auditor / Deps.Gate / Deps.Actor; build a FileAuditor in the CLI when the new audit config block is enabled (default off; path defaults to the XDG state dir), attributing the OS user as actor. - Test the file sink (JSONL, ok/error outcome, append) and the seam (a verb is audited; a gate-blocked op is neither run nor audited).
- Implement the app.Gate seam (added in the audit commit) as a prompting CLI gate driven by a profile's group policy: confirm_destructive makes the operator retype the profile name; require_2fa prompts for a TOTP code. A profile with no group, or a group with neither flag set, is allowed silently. The gate runs before the verb, so a denial aborts before any destructive work. - Add internal/twofactor: stdlib-only RFC 6238 TOTP (HMAC-SHA1, 30s, 6 digits) with Verify (±1 step skew) and Generate. No new dependency. - Add GroupConfig.totp_secret (a secret-ref, never plaintext) and resolve it through the existing secret resolver. require_2fa with no resolvable secret fails closed (blocks), never opens. - Build the gate in the CLI only when some group enforces a policy; prompt on stdin, report on stderr so prompts don't pollute stdout. - Test RFC 6238 vectors (current/adjacent/stale steps, formatting tolerance, bad secret) and the gate (no-group passthrough, name confirm match/mismatch, TOTP accept/reject, missing-secret blocks).
- Add internal/telemetry: a Recorder that plugs into the SAME audit Auditor seam (no new interception point) and accumulates per-op counts and error tallies, flushed as JSON. Off by default. - Privacy by construction: the Recorder reads only the op name and outcome from the audit Event — never profile, actor, target, or any data. A test asserts no identifying field reaches the file. - Add audit.Multi: fans one Begin/End out to several Auditor sinks, so the audit log and telemetry both observe the destructive-op seam without the app layer wiring two hooks. Nil sinks are skipped; all-nil collapses to a nil (no-op) Auditor. - Add a telemetry config block (enabled + path; XDG state default) and compose file-audit + telemetry via Multi in the CLI. - Known scope note (in code): audit.Handle.End carries only the error, not duration, so telemetry tracks counts/error-rate but not timing; carrying duration would mean widening the End signature — deferred. - Test aggregation (counts/errors), the privacy guarantee, empty-path disable, and Multi fan-out / all-nil collapse.
- Replace the schedule placeholder with a crontab manager. siphon does
NOT run a scheduler daemon — it maintains a delimited, siphon-owned
block in the user's crontab that invokes `siphon backup <profile>` on
a cron expression, delegating execution to the host's cron.
- Add internal/schedule: pure, I/O-free crontab-text editing (Add /
List / Remove) that touches only the managed block and preserves the
user's own crontab lines verbatim. Add reschedules an existing profile
in place rather than duplicating; removing the last entry drops the
block entirely.
- CLI: `schedule add <profile> --cron`, `schedule list`, `schedule
remove <profile>`, reading/writing the real crontab via `crontab -l`
/ `crontab -` ("no crontab" is treated as empty, not an error).
- Test the pure editor: add/list round-trip, in-place reschedule,
preservation of non-managed lines, no-op remove, empty cases.
- Replace the tunnel placeholder with a foreground SSH local-forward helper. `siphon tunnel <profile>` runs `ssh -N -L <local>:<dbhost>:<dbport> <bastion>` via the system ssh client (the user's ssh config, keys, and agent apply) and holds it open until Ctrl-C — delegation, not an SSH reimplementation or a daemon. - Add ProfileConfig.tunnel (bastion + optional local_port, defaulting to the DB port). A profile without tunnel.bastion gives a clear CodeUser error. - Use ExitOnForwardFailure=yes so ssh fails fast if the local bind is taken rather than opening a useless session; run under the command context so cancellation tears the tunnel down. - Update the stale stub test (schedule/tunnel are no longer "not implemented"): assert bare `schedule` shows its subcommands. - Test the pure ssh arg builder.
- Add docs/OPS.md covering audit log, 2FA/group gating, telemetry, schedule, and tunnel (config + behavior for each). - Update README roadmap row and command table; flip schedule/tunnel from "(Phase G)" placeholders to their real subcommands/usage. - Add the Phase G ops-suite entry to CHANGELOG.
|
Warning Review limit reached
More reviews will be available in 36 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds Phase G ops features: audit logging, destructive-operation gating, opt-in telemetry, TOTP verification, guarded backup/restore/sync/prune execution, recurring-backup schedule commands, an SSH tunnel command, and matching documentation updates. ChangesPhase G Ops Suite
Sequence Diagram(s)sequenceDiagram
participant Backup as "app.Backup"
participant GuardedOp as "guardedOp"
participant PromptGate as "promptGate"
participant Verify as "twofactor.Verify"
participant Record as "audit.Record"
participant Multi as "audit.Multi"
participant FileAuditor as "audit.FileAuditor"
participant Recorder as "telemetry.Recorder"
Backup->>GuardedOp: guardedOp(ctx, d, audit.OpBackup, profile, target)
GuardedOp->>PromptGate: Authorize(ctx, op, profile)
PromptGate->>Verify: Verify(secret, code, now)
PromptGate-->>GuardedOp: allow
GuardedOp->>Record: Record(ctx, d.Auditor, Event)
Record->>Multi: Begin(event)
Multi->>FileAuditor: Begin(event)
Multi->>Recorder: Begin(event)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
internal/audit/multi.go (1)
15-26: 🗄️ Data Integrity & Integration | 🔵 TrivialAvoid reusing the variadic backing array.
live := auditors[:0]compacts in place, soNewMulti(sinks...)can mutate the caller’s slice contents. Allocate a fresh slice to keep this exported helper side-effect free.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/audit/multi.go` around lines 15 - 26, NewMulti is compacting the variadic input in place by slicing auditors[:0], which can mutate the caller’s backing array when invoked with sinks.... Update the NewMulti helper to build live from a fresh slice instead of reusing auditors, while preserving the nil-filtering logic and the existing return behavior. Keep the change localized to NewMulti and the Multi constructor path so the exported helper remains side-effect free.internal/twofactor/totp_test.go (1)
13-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an independent RFC vector instead of only self-generated codes.
These tests compute expected codes with the same
generateimplementation under test, so truncation/modulus bugs can self-confirm. Add at least one fixed vector, e.g. counter 1 / Unix 59 should produce287082for the RFC test key.Example test addition
+func TestGenerate_RFCVector(t *testing.T) { + code, err := Generate(rfcSecret, time.Unix(59, 0)) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if code != "287082" { + t.Fatalf("Generate RFC vector = %q, want %q", code, "287082") + } +}Also applies to: 22-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/twofactor/totp_test.go` around lines 13 - 18, Add an independent RFC 6238 vector to the TOTP tests instead of deriving the expected code with generate in TestVerify_AcceptsCurrentStep and the related test cases. Use a fixed known value for the RFC test key (for example, counter 1 at Unix 59 should verify against 287082) and assert Verify returns that code so the test validates the implementation rather than self-confirming generate. Keep the change localized to the existing TestVerify_AcceptsCurrentStep and nearby Verify test helpers in totp_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/OPS.md`:
- Around line 77-89: Clarify the cron limitation in the `siphon schedule`
documentation block: scheduled entries created by `siphon schedule add`, `list`,
and `remove` ultimately invoke `siphon backup <profile>`, and that command still
goes through the confirmation/2FA gate with no unattended bypass. Update the
`siphon schedule` section in `docs/OPS.md` to note that gated profiles cannot be
safely run by cron unless that interaction is removed or a non-interactive path
is added in `backup`.
In `@internal/app/backup.go`:
- Around line 81-89: The backup flow leaves the audit record open when job
startup fails because guardedOp is called before Runner.Run and done(retErr)
only runs inside the job Func. Update the backup path in the Run/guardedOp
sequence to ensure the returned done callback is invoked if Runner.Run returns a
synchronous error, so the audit record is finalized even when the job never
starts.
In `@internal/app/sync.go`:
- Around line 63-66: The continuous and cross-engine sync paths bypass the
shared guard, so 2FA/confirmation gating and audit logging are only enforced on
the native sync route. Update Sync, RunCDC, and runCrossEngineSync so the shared
guardedOp flow is applied before any destructive sync work starts, or refactor
the guard into a common helper that all three paths must call. Make sure the
existing guardedOp/audit.OpSync behavior is reused consistently across these
entry points.
In `@internal/cli/gate.go`:
- Around line 82-84: The 2FA prompt in gate.go currently reads the code with the
normal stdin path, which can echo the TOTP value in interactive terminals.
Update the code in the gate flow around the existing bufio.NewReader/Verify path
to detect when g.in is a terminal and use a no-echo reader there, while
preserving the current g.in reader behavior for tests and non-interactive input.
Keep the existing twofactor.Verify(secret, line, g.now()) call and the prompt
flow intact.
- Around line 68-83: The confirm flow in promptGate.confirmName and
promptGate.confirmTOTP creates a new bufio.Reader for each prompt, which can
consume and discard piped input between destructive-name confirmation and 2FA.
Reuse a single buffered reader for both reads by storing it on promptGate or
passing it through both methods, and update both confirmName and confirmTOTP to
read from the shared reader so scripted input like profile name plus TOTP code
is preserved.
In `@internal/cli/schedule.go`:
- Around line 57-68: Validate the cron input in schedule.Add before calling
schedule.Add or writing the managed block, since it currently accepts any
non-empty string and can store expressions that schedule.List cannot round-trip.
Add a check in the schedule.Add flow to reject non-5-field cron specs such as
`@daily` or 6-field expressions, and return a user-facing errs.Error similar to
the existing --cron required validation. Use the existing schedule.Add,
schedule.List, and readCrontab path to keep the contract consistent.
- Around line 105-114: The empty-output fallback in the schedule listing logic
is too broad because it masks real system failures from exec.Command("crontab",
"-l").Output(). In the schedule list path, keep only the explicit ExitError
branch in listSchedule/its crontab read handling that checks for the known “no
crontab” message, and remove the len(out) == 0 shortcut so missing binaries and
other crontab errors still return the errs.Error with the system error details.
In `@internal/cli/tunnel.go`:
- Around line 40-48: Validate the tunnel port values in the tunnel command
before calling tunnelArgs: check prof.Tunnel.LocalPort and prof.Port for being
within the valid TCP port range and reject zero, negative, or out-of-range
values with a user-facing error instead of proceeding. Update the logic around
localPort selection and the tunnelArgs call in the tunnel open flow so invalid
ProfileConfig.Port or TunnelConfig.LocalPort never reaches ssh, and keep the
success message only for validated ports.
- Around line 52-55: The cmd.Run() error handling in tunnel logic is treating
the normal Ctrl-C/interrupt shutdown path as a tunnel failure. Update the branch
in the tunnel command execution to detect context cancellation or
interrupt-style ssh exits and return success instead of wrapping them in
errs.CodeSystem; only wrap genuine ssh failures with the existing errs.Error
path.
In `@internal/schedule/crontab.go`:
- Around line 33-35: The rendered cron command in Entry.render is interpolating
bin and e.Profile directly into a shell command, so update this path to
shell-escape or quote each argument before writing the crontab. Use the render
method as the fix point and ensure both the executable path and profile value
are treated as separate argv elements, or validate e.Profile against a safe
charset before formatting the command string.
In `@internal/telemetry/telemetry.go`:
- Around line 57-76: The persisted telemetry snapshot in recHandle.End can
regress because h.r.flush(snapshot) runs after h.r.mu is unlocked, allowing
concurrent End calls to write stale snapshots out of order. Fix this by
serializing the flush with the same lock (or by adding a dedicated write mutex
in the recorder path) so the snapshot captured in snapshotLocked is written to
disk before another End can overwrite it. Keep the change centered on
recHandle.End, snapshotLocked, and flush so on-disk counts remain monotonic.
---
Nitpick comments:
In `@internal/audit/multi.go`:
- Around line 15-26: NewMulti is compacting the variadic input in place by
slicing auditors[:0], which can mutate the caller’s backing array when invoked
with sinks.... Update the NewMulti helper to build live from a fresh slice
instead of reusing auditors, while preserving the nil-filtering logic and the
existing return behavior. Keep the change localized to NewMulti and the Multi
constructor path so the exported helper remains side-effect free.
In `@internal/twofactor/totp_test.go`:
- Around line 13-18: Add an independent RFC 6238 vector to the TOTP tests
instead of deriving the expected code with generate in
TestVerify_AcceptsCurrentStep and the related test cases. Use a fixed known
value for the RFC test key (for example, counter 1 at Unix 59 should verify
against 287082) and assert Verify returns that code so the test validates the
implementation rather than self-confirming generate. Keep the change localized
to the existing TestVerify_AcceptsCurrentStep and nearby Verify test helpers in
totp_test.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8d93878-9235-4025-889e-97c91bf9db01
📒 Files selected for processing (27)
CHANGELOG.mdREADME.mddocs/OPS.mdinternal/app/backup.gointernal/app/ops.gointernal/app/prune.gointernal/app/prune_test.gointernal/app/restore.gointernal/app/sync.gointernal/audit/audit.gointernal/audit/audit_test.gointernal/audit/file.gointernal/audit/multi.gointernal/cli/gate.gointernal/cli/gate_test.gointernal/cli/root.gointernal/cli/root_test.gointernal/cli/schedule.gointernal/cli/tunnel.gointernal/cli/tunnel_test.gointernal/config/config.gointernal/schedule/crontab.gointernal/schedule/crontab_test.gointernal/telemetry/telemetry.gointernal/telemetry/telemetry_test.gointernal/twofactor/totp.gointernal/twofactor/totp_test.go
- Apply post-review fixes across the ops suite. - sync: guard ALL non-CDC sync variants — the guardedOp call moved above the cross-engine branch, and RunCDC now guards itself, so 2FA/audit cover native, cross-engine, and continuous sync (previously only native). A security gap: --cross-engine bypassed 2FA. - crontab: shell-quote the rendered command's bin path and profile so a space or metacharacter in either cannot break or inject into the cron line cron runs via sh -c. Regression test with an injection payload. - gate: use one shared buffered reader across both prompts so scripted "name\ncode\n" is not split; read the TOTP code with echo disabled on a terminal (term.ReadPassword), falling back to the reader otherwise. - audit: finalize the record if job launch fails synchronously (launchGuarded) so an authorized-but-unstarted op is not left open. - schedule: reject non-5-field --cron up front (it would install but then vanish from list/remove); narrow readCrontab so only the known empty-crontab exit is treated as empty, not any zero-output failure. - tunnel: validate ports before building the forward; treat a context-cancelled (Ctrl-C) ssh exit as success, not a failure. - telemetry: flush under the lock so concurrent Ends keep the on-disk JSON monotonic. - docs: note that scheduled backups can't run gated profiles unattended.
Summary
Completes Phase G's remaining ops surface in one PR, structured as five self-contained feature commits + docs (reviewable commit-by-commit): audit log, 2FA/group gating, telemetry,
schedule, andtunnel.The three cross-cutting features (audit, 2FA, telemetry) share one interception seam —
guardedOp, added in the audit commit and wired into the destructive verbs (backup/restore/sync/prune) exactly once. 2FA layers on as a pre-check Gate; telemetry layers on as a secondaudit.Auditorsink viaaudit.Multi. Neither re-touches the verbs.Commits
feat(audit)— append-only JSONL audit log (who/what/when/outcome) via a newinternal/auditleaf + theguardedOpseam. Off by default (audit:config).feat(2fa)— profile-group gating:confirm_destructive(retype the profile name) andrequire_2fa(TOTP). Stdlib RFC 6238 (internal/twofactor), no new dep;totp_secretis a secret-ref; missing secret fails closed.feat(telemetry)— opt-in aggregate op counts/error tallies (internal/telemetry). Records op + outcome only — no identifying data (asserted by test). Composed viaaudit.Multi.feat(schedule)—siphon schedule add/list/remove: manages a siphon-owned block in the user's crontab (host cron runssiphon backup <profile>; no daemon). Pure crontab-text editor ininternal/schedule.feat(tunnel)—siphon tunnel <profile>: foregroundssh -Llocal-forward via a configuredtunnel.bastion, using the system ssh client.docs—docs/OPS.md, README, CHANGELOG.Design notes (decisions made for "just build")
scheduleandtunneldelegate to the OS (crontab, systemssh) rather than running a daemon or embedding an SSH library — idiomatic for a CLI helper, minimal dependency surface.audit.Handle.Endcarries only the error, not duration, so telemetry tracks counts/error-rate; carrying duration would mean widening the seam (noted in code, deferred).Testing
make test— 20 packages pass;make lint— 0 issues;go vet -tags=integration ./...clean.Multifan-out, the audit file sink, and the app seam (a verb is audited; a gate-blocked op is neither run nor audited).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes